Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

main: add --verbose,-v persistent flag that increases verbosity #790

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Jan 9, 2025

This commit adds --verbose,-v which will increase the verbosity
of logrus and also switch the --progress to "verbose". This is
addressing the feedback we got in
#765
and a followup for #776

The new -v clashes unfortunately with cobras default for version,
so there is no single dash flag for version anymore. Most unix tools
(e.g. cp,rsync,mv,curl,ssh,tar) use "-v" for "--verbose" so IMHO we
should follow suite. Unfortuantely there is no consistency in linux,
e.g. git,gcc are counter examples where it means version). I would
still go with -v for verbose as ssh,tar,curl are probably used
more often to get verbose output.


main,test: tweak cmdVersion

This commit tweak the new and very nice functionality of cmdVersion (see commit message for details)


(note that the above commit is not strictly needed, because of the slightly inflexible way of how cobra handles the version I had to look at this function and did some quick tweaks but I could drop it from this PR and/or drop it entirely)

Thanks to Ondrej for the suggestion about -v/--verbose

This comit tweak the new and very nice functionality of
cmdVersion in the following way:
- Rename to versionFromBuildInfo as it is no longer a "cmd*" (i.e.
  it no longer takes a cobra.Command)
- Use switch/case as it's slightly more compact than if/else
- Just build the string directly instead of using a list (slightly
  shorter)
- Change "build_status: ok" to "build_tainted" with a boolean value
  to ensure this is easier to parse in yaml (and more descriptive
  as "status" is quite generic and may mean many things to people).
- Extend the test_bib_version to test for the full strings prefixes
  in test_opts.
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanups. The ambiguity of -v across the linux stack is sad, but I agree that having it for verbose, and then --version and version for displaying the version info is probably the better choice (100% subjective ofc). We should make sure that image-builder-cli uses the same convention.

I requested changes, because -v should be documented in the README. ;)

This commit adds `--verbose,-v` which will increase the verbosity
of logrus and also switch the --progress to "verbose". This is
addressing the feedback we got in
osbuild#765
and a followup for osbuild#776

The new `-v` clashes unfortunately with cobras default for version,
so there is no single dash flag for version anymore. Most unix tools
(e.g. cp,rsync,mv,curl,ssh,tar) use "-v" for "--verbose" so IMHO we
should follow suite. Unfortuantely there is no consistency in linux,
e.g. git,gcc are counter examples where it means version). I would
still go with -v for verbose as ssh,tar,curl are probably used
more often to get verbose output.
@mvo5
Copy link
Collaborator Author

mvo5 commented Jan 10, 2025

Nice cleanups. The ambiguity of -v across the linux stack is sad, but I agree that having it for verbose, and then --version and version for displaying the version info is probably the better choice (100% subjective ofc). We should make sure that image-builder-cli uses the same convention.

I requested changes, because -v should be documented in the README. ;)

Thanks, I added it now to the README, we should probably do a followup as well that documents more options in the README, its a bit out of sync with reality. I can look after this one here is merged.

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, a README cleanup would be nice. Thanks! :)

@mvo5 mvo5 added this pull request to the merge queue Jan 10, 2025
Merged via the queue into osbuild:main with commit d61eaaa Jan 10, 2025
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants